Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce add_labels and add_tags processors #9973

Merged
merged 8 commits into from
Jan 11, 2019
Merged

Introduce add_labels and add_tags processors #9973

merged 8 commits into from
Jan 11, 2019

Conversation

urso
Copy link

@urso urso commented Jan 9, 2019

Introduce add_fields and add_tags processors. The fields/tags settings will be marked deprecated starting with 6.7.

Replace custom processors in pipeline/processors.go with new definitions.

Cleanup pipeline/processors.go by removing dead code.

Fix recent regressions from beat -> agent renaming potentially triggering a panic due to concurrent map updates.

@urso urso requested a review from a team as a code owner January 9, 2019 19:08
@urso urso added in progress Pull request is currently in progress. libbeat labels Jan 9, 2019
@urso urso force-pushed the fields-tags-processor branch from 193c6fc to 1b9de01 Compare January 9, 2019 19:18
kaiyan-sheng
kaiyan-sheng reviewed Jan 9, 2019
View reviewed changes
libbeat/processors/actions/add_tags_test.go
}

for name, test := range cases {
test := test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great example for me. Thanks! Just curious, is this test := test necessary here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends. I tend to put it here so to bind the name locally in case I enable t.Parallel(). functions/go-routines in a for-block accessing a loop variable will see the value being changed otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handy tricks, I should use it more often.

ruflin
ruflin reviewed Jan 10, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see these 2 new processor. Left some feedback related to ECS.

If we remove the feature in 7.0, would be perhaps provide some migration code to modify the config to use the processors instead? Not sure how comment the usage of these 2 is, but I assume not rare.

libbeat/processors/actions/add_fields.go Outdated Show resolved Hide resolved
libbeat/processors/actions/add_fields.go Outdated Show resolved Hide resolved
libbeat/processors/actions/add_tags.go Show resolved Hide resolved
@urso
Copy link
Author

urso commented Jan 10, 2019

Note: I will add documentation later. There is some more cleanup/renaming of other processors I want to do as well first.

@urso urso changed the title [WIP] Introduce add_fields and add_tags processors Introduce add_labels and add_tags processors Jan 10, 2019
@urso urso added review needs_backport PR is waiting to be backported to other branches. and removed in progress Pull request is currently in progress. labels Jan 10, 2019
@urso urso force-pushed the fields-tags-processor branch from d1ded38 to acc6372 Compare January 10, 2019 17:25
@ph ph self-requested a review January 10, 2019 17:32
ph
ph approved these changes Jan 11, 2019
View reviewed changes
libbeat/processors/actions/add_tags_test.go
}

for name, test := range cases {
test := test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handy tricks, I should use it more often.

ruflin
ruflin reviewed Jan 11, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the following config work?

processors:
  add_labels:
    labels:
      foo.bar: 17

I'm mainly concerned about the . in the key. If not, is that something we want to support? Would be nice to have some system tests to confirm this works or not.

@urso
Copy link
Author

urso commented Jan 11, 2019

Will the following config work?

What do you mean by work. It works the same current fields settings work. You configuration will produce a document saying:

{
  ...
  "labels": {
    "foo": {
      "bar": 17
    }
  },
  ...
}

The way we read config files one can only choose between dedot or flat dottet output always. The information whether a dot was used in the config or not is lossed thanks to the config file parser.

@andrewkroh andrewkroh mentioned this pull request Jan 11, 2019
Closed
20 tasks
@urso urso merged commit 14373c7 into elastic:master Jan 11, 2019
@ruflin ruflin mentioned this pull request Jan 14, 2019
Closed
@urso urso removed the needs_backport PR is waiting to be backported to other branches. label Jan 31, 2019
@urso urso deleted the fields-tags-processor branch February 19, 2019 18:32
@andrewkroh andrewkroh mentioned this pull request Feb 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaiyan-sheng kaiyan-sheng kaiyan-sheng left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@ruflin ruflin ruflin left review comments

@kvch kvch kvch approved these changes

@ph ph ph approved these changes

Assignees
No one assigned
Labels
libbeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@urso @ph @ruflin @kvch @houndci-bot @kaiyan-sheng